feat(codex): restore LazyCodex role models from a change history - #7693

Merge Risk: Low · previous review 0efe4

Restore failures now report a partial result, and the guide identifies the correct file. The remaining issues affect history guidance and localized display; the change is mergeable with those corrections as follow-up.

Pre-merge checks: 4

Check name	Status	Explanation
Description Check		Check skipped - CodeRabbit’s high-level summary is enabled.
Title check		The title clearly and concisely describes the main change: adding LazyCodex role-model change history with restore support. This matches the history storage, API, dashboard, localization, documentatio…
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.

Full details: Title check

Explanation

The title clearly and concisely describes the main change: adding LazyCodex role-model change history with restore support. This matches the history storage, API, dashboard, localization, documentation, and test changes.

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

_{Comment @coderabbitai help to get the list of available commands.}

✅ Deterministic PR hygiene checks passed.

Maintainer triage review (round 2, against dev 0aeaf0c), head e989e18.

1. Restore can apply a partial batch. src/codex/role-model-history.ts:69 truncates individual rows at 50, so an Apply All batch that crosses the boundary is partially restored by gui/src/pages/integrations/LazyCodexRoleHistory.tsx:51-55. Please return complete batches (or mark incomplete batches and disable Restore) and add a boundary regression.
2. Documented location: docs-site/src/content/docs/guides/integrations.md:843 should name integrations/codex-agent-roles.jsonl; integrationsDir() adds that subdirectory (src/integrations/ownership.ts:145-146).
3. feat(codex): tier role auto-assign candidates by measured capability in the role's task domain #7624 shares the role routes, structure/subagents.md and the integrations guide; coordinate with it. Executable CI is awaiting maintainer approval.

⏳ DRAFT

• review readiness checklist open (0/4 boxes ticked).

What to do

• Tick all four boxes in the PR description once you're done (currently 0/4).

Review readiness checklist

• ⬜ Required local validation passed; commands, results, and any full-suite exception are documented.
• ⬜ I pushed my PR to a recent dev commit (at most 10 behind; a maintainer may still ask for the exact tip before merge).
• ⬜ I resolved all correct Codex and CodeRabbit findings.
• ⬜ My PR is ready for review.

0/4 boxes ticked.

Automatic draft conversion failed. Please convert this pull request to a draft manually until every box above is ticked.

Hygiene

✅ Deterministic PR hygiene checks passed.

Maintainer review, round 2 (head 1334f37, against dev 50afcec). Thanks: whole-batch history listing with the 51-row boundary regression, and the corrected integrations/codex-agent-roles.jsonl path both look right.

One remaining item before this can be considered:

1. P2: an unreadable history file takes down role management. src/codex/role-model-history.ts:70 calls readFileSync without handling read errors, so an existing history file that returns EACCES (or any read error) makes GET /api/codex-agent-roles fail via src/server/management/codex-agent-role-routes.ts:47, and gui/src/pages/integrations/LazyCodexRoleModels.tsx:161 then replaces the whole role editor with an error. History is optional: please degrade to "history unavailable" while keeping role editing working, and add an injected read-failure regression.

Smaller notes: structure/subagents.md:304 still says "the newest 50 rows"; it should mention the whole-batch expansion. #7624 also touches the role routes, structure/subagents.md and the integrations guide; the edits are in different sections today, but please note the coordination in the PR. Persistence touches Windows filesystem behaviour, so a maintainer will run the full-platform CI before integration.

Thanks for round 2. Both items are in 163d95cf6 and 93cbc4a8e, on a head rebased onto current dev:

1. Unreadable history. GET /api/codex-agent-roles now catches a history read failure, logs it, and answers with history: [] and historyUnavailable: true; the role list and saves are unaffected. The dashboard shows a notice in place of the history list, translated in all eleven catalogs. The new route test puts a directory where codex-agent-roles.jsonl belongs, so the read fails with EISDIR on every platform, and checks that the list still answers 200 and a save still writes the TOML; with the route change reverted it fails. A GUI test checks the notice and that the role table stays editable.
2. structure/subagents.md:304 now says the newest 50 rows are widened to whole batches, and that an unreadable file sets historyUnavailable. The integrations guide says the same in one sentence.

The coordination with #7624 is noted in the PR body: both touch the role routes, structure/subagents.md and the integrations guide in different sections, with no ordering between them.

Verified at 93cbc4a8e with Bun 1.4.3: the route tests 20 pass, the four GUI test files 103 pass, bun run typecheck, bun run lint:gui and the GUI tsc --noEmit exit 0, and structure:check, privacy:scan and the file-size ratchet pass.

Thanks for the release review. All three items are fixed at a068b9de4, each in its own commit on a head rebased onto current dev; each new test failed before its fix.

1. History crosses Codex homes (f4a15f8e3). Each row now stores the canonical Codex home, and listing and Restore read only rows from the current home. Restore sends the batch it restores; the server answers 409 restore_not_in_history unless this home's history holds that batch for the role with exactly those earlier values. A two-home route test covers the overwrite your probe found.
2. Unbounded growth and reads (dc74dd7ce). The file has a 256 KiB budget: an append past it rewrites the file atomically, keeping the newest whole batches within 128 KiB. Reads take only the tail (at most 256 KiB and 1000 rows) and drop the batch the cut split. A batch holds at most 50 rows (409 batch_full before the TOML is touched), and a listing returns whole batches, newest first, never more than 50 rows, so a reused batch id can no longer return 120.
3. No crypto.randomUUID (a068b9de4). Apply all and Restore use the existing LAN-safe newClientId() from gui/src/pages/claude-code-types.ts and clear busy in finally; tests cover a missing and a throwing randomUUID for both actions.

Verified at this head: the role-route, auto-assign-route and CLI parity tests 139 pass / 0 fail; the three GUI test files 106 pass / 0 fail; bun run typecheck, GUI tsc --noEmit, lint:gui, GUI lint:i18n and the GUI build exit 0; structure:check and privacy:scan pass. structure/subagents.md and the integrations guide describe the home scoping and the bounds.

The 256 KiB / 1000-row budget and the two 409 codes are my choices; happy to change either.

Thanks for the fast turnaround. Release class for this PR: POST-RELEASE. It lands after v2.93.0 is published on main, not before.

An independent round-2 review of a068b9de4 confirmed two of the three round-1 fixes with its own probes:

• A stale Restore against a second Codex home now returns 409 restore_not_in_history and leaves that home's role bytes unchanged.
• Apply all and Restore work without randomUUID, and when it throws, and they release busy state.

Auth, CSRF and atomic writes are unchanged. All 11 locales are complete, and the size and layout guards pass (109 / 0 focused).

Two new blocking problems remain, both in the history bounds:

1. Restore invalidates its own remaining writes (src/codex/role-model-history.ts:190, used at src/server/management/codex-agent-role-routes.ts:153). Restore authorization searches the 50-row display listing. When the source batch is a full 50-row batch, restoring the first role appends a newer row that pushes the whole source batch out of that listing. The next role's PUT then returns 409 restore_not_in_history, leaving the batch half-restored and impossible to retry. A probe got 200 on the first PUT and 409 on the second. Validate against the retained complete history rather than the paginated view, and keep the source batch eligible for the whole Restore.
2. Compaction breaks batch integrity and resets the cap (role-model-history.ts:69, :77, :149, :180). Compaction can drop the earlier rows of a batch that is still being reused. Later writes then see a smaller count and keep being accepted, and Restore treats an intermediate value as the original. A probe sent 120 PUTs with one batch id and 4,000-character model ids: all 120 succeeded with no batch_full, only 23 rows remained, and the original preimage was gone. An interleaved-batch probe offered middle as the Restore target instead of original. One fix: keep a durable per-batch count and earliest preimage across retention. Another: close or expire a batch at compaction and reject reuse. Either way, never offer a retained fragment as a complete batch.

When these are fixed with regressions, it gets a round-3 review and merges after the release.

Thanks for the second pass; both history-bound blockers are fixed at b063fb01b, one commit each, and each new test failed before its fix.

1. Restore invalidated its own remaining writes (e85982ee2). restorableSetting now checks this Codex home's whole retained history instead of the 50-row listing, so a Restore's own rows can no longer push its source batch out of reach. A row written by a Restore also records the batch it restores, and compaction keeps that batch whole while the Restore's rows are kept, so a compaction between two roles cannot drop it either. Two route tests restore both roles of a full 50-row batch, and restore across a compaction that the first restored row triggers. At a068b9de4 both returned 200 and then 409 restore_not_in_history; now both roles are back byte for byte.
2. Compaction broke batch integrity and reset the cap (b063fb01b). I combined your two options in the form that needs no side store. Each row stores its number in its batch, which is the durable count: it lives in the row itself, so compaction cannot reset it, and every read drops a batch missing any row number, so a retained fragment is never listed or offered to Restore. The server also hands out batch ids now. A write without batch starts one and the response names it, and a write may join a batch only while the retained history still holds it whole; otherwise it gets 409 batch_closed, so an id compaction dropped (or one the server never issued) cannot start over with a middle value as its first. That keeps the earliest preimage correct with no copy kept outside retention. Apply all and Restore send no batch with their first write and reuse the id its response returns, and the CLI never sends one. Your probes as tests, against e85982ee2: 120 PUTs on one batch id with 4,000-character models were all accepted (now at most 50, the rest batch_full or batch_closed); a compacted id was accepted again (now batch_closed, role file untouched); an interleaved batch whose row 1 was gone was listed, and Restore offered its middle value (now neither listed nor restorable).

Verified at this head: the role-route, auto-assign-route and CLI parity tests 144 pass / 0 fail (139 before plus five new); the three GUI test files 106 pass / 0 fail; bun run typecheck, GUI tsc --noEmit, lint:gui, GUI lint:i18n and the GUI build exit 0; structure:check, privacy:scan and the file-size ratchet pass. In one full-suite run the parity test every GUI management endpoint belongs to a documented CLI resource timed out at 6.4 s against its 5 s limit; alone it passes in 46 ms, and the rerun of all three files was 144 / 0. structure/subagents.md describes the server-issued ids, batch_closed, the per-row number and the retained-history Restore check. The integrations guide's user-facing description did not change.

The new 409 code batch_closed and the batch field on a recorded PUT's response are my choices; happy to rename either.

Independent round-3 review of d1f6319d7: MUSTFIX, 2 blocking findings. Thanks for the quick turnaround on round 2. Most of it holds up under independent probes: a full 50-row Restore across both roles, an ordinary mid-Restore compaction, 120 PUTs on one id (50 accepted, 70 rejected), unknown and compacted ids closed, concurrent sessions getting distinct ids and row numbers, home isolation, bounded reads, missing or throwing randomUUID, and atomic-write retry. Existing clients that omit batch keep their contract. Locale parity, layout, size caps, typecheck, structure, GUI lint/build and privacy all pass, and the head merges cleanly with dev.

1. High: Restore can still invalidate its own remaining writes at the retention boundary (src/codex/role-model-history.ts:84). Compaction reads the bounded tail after the append, and that append can push the source batch's first row out of the window. wholeBatches then drops the source before the preservation logic sees it.
   • Probe: a real server-issued two-role batch, interleaved with 547 filler rows, was complete and listed at 262,080 bytes. The first Restore PUT returned 200 and compacted history to 131,030 bytes; the second returned 409 restore_not_in_history, and the second role stayed unchanged.
   • Repair direction: preserve the complete source batch before the append changes the bounded window, compact from that snapshot plus the new row, and count protected sources against both retention limits.
2. Medium: a torn history suffix closes the next Apply-all batch (src/codex/role-model-history.ts:67). Appending after an interrupted JSONL row concatenates the new record onto invalid JSON.
   • Probe: with history ending in {"at":"interrupted, the first PUT returned 200 with a batch id, but parsing discarded its row. The next role's PUT with that id returned 409 batch_closed, leaving a half-applied batch.
   • Repair direction: terminate or discard an unterminated suffix before appending, and add a restart-recovery regression showing that a returned batch id stays joinable.

The documentation's source-retention guarantee also needs to match the fix for item 1.

To keep this moving now that v2.93.0 is out, a maintainer will push a fix for both items to this branch, with regression tests that fail first, and send it through another independent review. @LilMGenius, please hold off pushing to the branch until that lands. Review of the maintainer commits is very welcome.

Thanks for taking both round-3 items, and sorry for pushing after you asked me to hold off. The push only rewrote history; no file changed.

• Every tree is identical, including the tip: a99745788 has the same tree as your merge d05d14138.
• Your two commits keep you as author and committer, with their original dates. They are now 36172ec3e and a99745788.
• The merge of d9f594b01 is linearized: all 16 commits are replayed onto that same dev commit, with no conflicts.
• My 14 commits got back their first committer dates and their verified signatures. An automated rebase had restamped and re-signed them with an unknown key.

Please reset your local branch to a99745788 before your next push, so the old SHAs don't come back.

Review of your fixes: I ran bun test tests/server/codex-role-history-recovery.test.ts tests/server/codex-agent-role-routes.test.ts (Bun 1.4.2, Windows) at two points:

The 5 failures on 8d08e192e are the three byte, row and budget retention-boundary cases for item 1, plus the two torn and complete unterminated-suffix restart cases for item 2. So each regression fails without its fix. The docs change in integrations.md and structure/subagents.md states the source-retention guarantee the way item 1 now implements it. I have nothing to add from my side.

Maintainer integration into dev (MAINTAINERS.md, "Review and merge policy"). This is maintainer integration, not an approving review.

• Exact head: d9d343f0a13e6ddd2a745512eedd7d5c6c443594, a normal merge of dev into the round-4 PASS head a99745788; no conflict with current dev.
• Exact-head PR CI: Cross-platform CI 40584275586, Catalog async contracts 40584275449, React Doctor 40584275540, enforce-target: all success.
• Review: rounds 1-3 findings fixed (Codex-home isolation, bounded history, LAN-safe ids, history-bound retention and torn-suffix handling); round 4 PASS with 0 blocking.

Thank you @LilMGenius for the feature and the quick turnarounds.

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.